signal: custom exit code status for interceptor #5636

DarthBenro008 · 2021-08-17T18:47:07Z

fixes lnd should exit with no-success error code when automatically shut down due to bitcoind problems #5625

Outline and Challenge

This issue was required lnd to exit with status code 1 where there was an request to shutdownRequestChannel

Lines 100 to 102 in 52767a7

    
           case <-c.shutdownRequestChannel: 
        
           	log.Infof("Received shutdown request.") 
        
           	shutdown()

The challenge was that we had to do an os.exit(1) after an graceful shutdown.

An graceful shutdown occurs when all the deferred go function in the Main() function clean up, but there is no way that we can pass the data or tell the function that cleans up last to exit with a particular status code.

Approach

I pulled the defer function of Main() outside from lnd package and put it under cmd/lnd.go where the Main() get's called (giving us parental access to shutdownInterceptor) and added an additional parameter to Interceptor struct so that we can pass data when an graceful shutdown has occurred.

I am passing a pointer reference in-order to retain the modifications (setting exit code) in the Intercept struct in goroutines.

Before	After

Steps to test this PR

run bitcoind server
run the following ./lnd-debug --rpclisten=localhost:10001 --listen=localhost:10011 --restlisten=localhost:8001 --debuglevel=debug --healthcheck.chainbackend.backoff=1s --healthcheck.chainbackend.interval=1m0s --healthcheck.chainbackend.timeout=3s
create a wallet ./lncli-debug --rpcserver=localhost:10001 --macaroonpath=data/chain/bitcoin/simnet/admin.macaroon unlock
Press ctrl+c and shutdown your bitcoind server
Wait for 1 min, lnd will exit gracefully with an exit status code 1

Pull Request Checklist

DarthBenro008 · 2021-08-17T18:48:04Z

Hi @carlaKC ! I have solved the issue, ensured the CI checks pass with a PR on personal fork 😃

cmd/lnd/main.go

carlaKC

Thanks for the PR! Looking good

cmd/lnd/main.go

signal/signal.go

cmd/lnd/main.go

DarthBenro008 · 2021-08-18T13:02:43Z

Hey @carlaKC Implemented and amended the suggested changes!

DarthBenro008 · 2021-08-19T03:51:00Z

Force pushed from 1057f6d to 1764854 to fix a lint issue reported by Travis.

carlaKC

Thanks for the quick turnaround!

Two things:

Why not just pass a pointer to the Interceptor around?
If we're going to use a channel for the exit code, it needs to always have a value sent in so that the caller can read the exit code value without getting blocked.

signal/signal.go

carlaKC · 2021-08-20T09:04:20Z

mobile/bindings.go

@@ -75,7 +75,7 @@ func Start(extraArgs string, rpcReady Callback) {

 	// Load the configuration, and parse the extra arguments as command
 	// line options. This function will also set up logging properly.
-	loadedConfig, err := lnd.LoadConfig(shutdownInterceptor)
+	loadedConfig, err := lnd.LoadConfig(*shutdownInterceptor)


Why do we want to dereference here? Can't we just pass a pointer in?

I initially tried passing it directly to figure out the LoadConfig does not accpet and reference to Interceptor. I was not sure which all aspects of LND would be using the loadConfig as it seems very important to parse the user configs, hence i decided to go the safe way and passed a de-referenced interceptor

lnd/config.go

Line 569 in 9e8b9cc

func LoadConfig(interceptor signal.Interceptor) (*Config, error) {

You can bump LoadConfig to accept a pointer 👍

This commit adds an ability to interceptor to have custom exit status-codes for interceptor that allows to exit after an graceful exit Signed-off-by: DarthBenro008 <[email protected]>

Signed-off-by: DarthBenro008 <[email protected]>

DarthBenro008 · 2021-08-20T10:16:58Z

Hey @carlaKC , I have implemented the recommended changes and have resolved the conversations 😄

carlaKC

Shaping up nicely!

carlaKC · 2021-08-24T07:41:53Z

signal/signal.go

 	if !atomic.CompareAndSwapInt32(&started, 0, 1) {
-		return Interceptor{}, errors.New("intercept already started")
+		return &Interceptor{}, errors.New("intercept already started")


can just return nil here, safe to assume that it won't be used when there's a non-nil error :)

carlaKC · 2021-08-24T07:42:04Z

cmd/lnd/main.go

@@ -34,9 +34,10 @@ func main() {
 	// Call the "real" main in a nested manner so the defers will properly
 	// be executed in the case of a graceful shutdown.
 	if err = lnd.Main(
-		loadedConfig, lnd.ListenerCfg{}, shutdownInterceptor,
+		loadedConfig, lnd.ListenerCfg{}, *shutdownInterceptor,


still dereferencing here?

carlaKC · 2021-08-24T07:45:56Z

signal/signal.go

@@ -34,20 +34,24 @@ type Interceptor struct {

 	// quit is closed when instructing the main interrupt handler to exit.
 	quit chan struct{}
+
+	// exitCode is the exit code status when the main interrupt handler exits
+	exitCode chan int


Worth adding a comment here that we must set a value here, otherwise the GetExitCode function will block.

carlaKC · 2021-08-24T07:46:58Z

signal/signal.go

 			shutdown()

 		case <-c.quit:
 			log.Infof("Gracefully shutting down.")
 			close(c.shutdownChannel)
 			signal.Stop(c.interruptChannel)
+			// Adding exit status code 0 as this will quit after graceful shutdown
+			c.setExitCode(0)


Do we need to set an exit code here? I think we can only hit this case if we've called shutdown in one of the other cases?

We can't send multiple values into the channel otherwise we'll end up blocked. I think it's worth adding a comment above <-c.quit that we only reach this case once shutdown has been called.

carlaKC · 2021-08-24T07:47:43Z

mobile/bindings.go

@@ -75,7 +75,7 @@ func Start(extraArgs string, rpcReady Callback) {

 	// Load the configuration, and parse the extra arguments as command
 	// line options. This function will also set up logging properly.
-	loadedConfig, err := lnd.LoadConfig(shutdownInterceptor)
+	loadedConfig, err := lnd.LoadConfig(*shutdownInterceptor)


You can bump LoadConfig to accept a pointer 👍

carlaKC · 2021-11-12T06:46:01Z

@DarthBenro008 any updates here?

lightninglabs-deploy · 2021-12-27T17:04:35Z

@DarthBenro008, remember to re-request review from reviewers when ready

carlaKC · 2022-01-03T06:31:03Z

Closing due to inactivity.

mohamedawnallah · 2024-04-17T18:33:27Z

Replaced by #8659

Roasbeef added this to the v0.14.0 milestone Aug 17, 2021

Roasbeef added error codes linux labels Aug 17, 2021

Roasbeef reviewed Aug 17, 2021

View reviewed changes

cmd/lnd/main.go Outdated Show resolved Hide resolved

DarthBenro008 requested a review from Roasbeef August 18, 2021 02:49

carlaKC reviewed Aug 18, 2021

View reviewed changes

cmd/lnd/main.go Outdated Show resolved Hide resolved

signal/signal.go Outdated Show resolved Hide resolved

signal/signal.go Show resolved Hide resolved

cmd/lnd/main.go Outdated Show resolved Hide resolved

DarthBenro008 force-pushed the fix/#5625 branch from be526ce to 1057f6d Compare August 18, 2021 13:00

DarthBenro008 requested a review from carlaKC August 18, 2021 13:01

DarthBenro008 force-pushed the fix/#5625 branch from 1057f6d to 1764854 Compare August 19, 2021 03:49

carlaKC reviewed Aug 20, 2021

View reviewed changes

DarthBenro008 added 2 commits August 20, 2021 15:45

feat: custom exit status-code for lnd interceptor

a8994dc

This commit adds an ability to interceptor to have custom exit status-codes for interceptor that allows to exit after an graceful exit Signed-off-by: DarthBenro008 <[email protected]>

mobile: pass de-referenced variable in mobile bindings

37a76a1

Signed-off-by: DarthBenro008 <[email protected]>

DarthBenro008 force-pushed the fix/#5625 branch from 1764854 to 37a76a1 Compare August 20, 2021 10:16

DarthBenro008 requested a review from carlaKC August 20, 2021 14:34

carlaKC reviewed Aug 24, 2021

View reviewed changes

Roasbeef added the P3 might get fixed, nice to have label Aug 31, 2021

Roasbeef modified the milestones: v0.14.0, v0.15.0 Sep 29, 2021

Roasbeef removed their request for review September 29, 2021 03:46

carlaKC closed this Jan 3, 2022

mohamedawnallah mentioned this pull request Apr 17, 2024

signal: manage shutdown requests with status codes #8659

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signal: custom exit code status for interceptor #5636

signal: custom exit code status for interceptor #5636

DarthBenro008 commented Aug 17, 2021

DarthBenro008 commented Aug 17, 2021 •

edited

Loading

carlaKC left a comment

DarthBenro008 commented Aug 18, 2021

DarthBenro008 commented Aug 19, 2021

carlaKC left a comment

carlaKC Aug 20, 2021

DarthBenro008 Aug 20, 2021

carlaKC Aug 24, 2021

DarthBenro008 commented Aug 20, 2021

carlaKC left a comment

carlaKC Aug 24, 2021

carlaKC Aug 24, 2021

carlaKC Aug 24, 2021

carlaKC Aug 24, 2021

carlaKC Aug 24, 2021

carlaKC commented Nov 12, 2021

lightninglabs-deploy commented Dec 27, 2021

carlaKC commented Jan 3, 2022

mohamedawnallah commented Apr 17, 2024

	case <-c.shutdownRequestChannel:
	log.Infof("Received shutdown request.")
	shutdown()

signal: custom exit code status for interceptor #5636

signal: custom exit code status for interceptor #5636

Conversation

DarthBenro008 commented Aug 17, 2021

Outline and Challenge

Approach

Steps to test this PR

Pull Request Checklist

DarthBenro008 commented Aug 17, 2021 • edited Loading

carlaKC left a comment

Choose a reason for hiding this comment

DarthBenro008 commented Aug 18, 2021

DarthBenro008 commented Aug 19, 2021

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarthBenro008 commented Aug 20, 2021

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Nov 12, 2021

lightninglabs-deploy commented Dec 27, 2021

carlaKC commented Jan 3, 2022

mohamedawnallah commented Apr 17, 2024

DarthBenro008 commented Aug 17, 2021 •

edited

Loading